chore: add docs, fail fast on execute for structural flawed solutions, make ScoreAnalysis not fail fact on structural flawed input - #2609
Conversation
Also add a fail-fast when executing a move that produced a structurally flawed solution.
…ad of raising an exception
There was a problem hiding this comment.
Leaving some comments.
Your comment about keeping the graph forever is noted. It is an implementation downside of a requirement which makes sense for users; we take the pain. Going forward, maybe we refactor the graph algorithm; it is fast becoming marginal, maybe we do not need all the incrementality anymore. Anyway, not for now.
| * Return a collection of {@link ai.timefold.solver.core.api.domain.entity.PlanningEntity} | ||
| * that have inconsistent shadow variables. | ||
| */ | ||
| Collection<Object> getInconsistentEntities(); |
There was a problem hiding this comment.
Can we separate this into individual loops?
IMO that would make some sense - it would allow for better debugging of what is looping.
There was a problem hiding this comment.
Also, I would prefer if this returned a SequencedSet - clearer semantics, without having to explain.
There was a problem hiding this comment.
It cannot be sequenced; loops are strongly connected components, and are not always of the form x1 -> x2 -> ... -> x1. There may be shortcuts:
x1 -> x2 -> x3 -> x4 -> x1,x1 -> y -> x1andx2 -> y.Sequencedimplies an ordering which is not possible for strongly connected components -- it is literally an "impossible to order" situlation.
There was a problem hiding this comment.
I am not asking for any meaningful ordering. I am asking for reproducible ordering - the difference between HashSet and LinkedHashSet. Is that not possible?
There was a problem hiding this comment.
You can have reproducible ordering without exposing the type as Sequenced; the actual instance is LinkedHashSet.
triceo
left a comment
triceo
left a comment
There was a problem hiding this comment.
We have a naming issue to solve.
(Also, a bit of a confusion in terms.)
Other than that, I think we're good.
| * @param variableName The variable on the entity. | ||
| */ | ||
| @NullMarked | ||
| public record EntityVariablePair(Object entity, String variableName) { |
There was a problem hiding this comment.
This will lead to a very verbose JSON.
I question if we need the variable information at all.
If we do, then arguably we can list entities per variable, as opposed to listing the variable with every entity.
There was a problem hiding this comment.
If you want the individual loops, you need the variables, and the loop might be entity1:a -> entity2:b -> entity1:c.
…, make ScoreAnalysis not fail fact on structural flawed input (#2609)
2 participants
For the record, I strongly dislike this change to
ScoreAnalysis(returning with structural flaws instead of throwing an exception), and it will force the old implementation of the graph to live forever in some ways.